-
-
Notifications
You must be signed in to change notification settings - Fork 0
Layers panel/jan26 updates shorterm #272
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
| {/* Potentially could be restored. Remove if still not restored by 2025-03-01 */} | ||
| {/* <ShapeSelector /> | ||
| <Separator /> | ||
| <FillSelector /> */} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
suggestion: remove this
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
This PR enhances the layers panel with comprehensive updates to improve user experience when managing map layers, markers, and data visualizations.
Changes:
- Consolidated members and markers into a unified markers control panel
- Added custom color picker for categorical data visualization
- Implemented context menus for renaming, hiding, and deleting layers/markers/turfs/data sources
- Replaced hover menus with right-click context menus for better UX
- Added empty state buttons for adding layers with dropdown options
- Improved legend display for categorical data with color swatches
- Added column filtering UI for "highest value" visualizations
- Updated icon library (lucide-react) and replaced custom VectorSquare icon
Reviewed changes
Copilot reviewed 33 out of 34 changed files in this pull request and generated 13 comments.
Show a summary per file
| File | Description |
|---|---|
| src/server/trpc/routers/dataSource.ts | Added name field to update and conditional import triggering |
| src/server/stats/index.ts | Added includeColumns parameter with column filtering logic and float casting |
| src/server/models/MapView.ts | Added categoryColors and includeColumnsString schema fields |
| src/components/DeleteConfirmationDialog.tsx | New reusable delete confirmation dialog component |
| src/app/map/[id]/hooks/useLayers.ts | Added marker visibility handling logic |
| src/app/map/[id]/components/controls/MarkersControl/* | Refactored to use context menus and added delete confirmations |
| src/app/map/[id]/components/controls/TurfsControl/* | Replaced hover menus with context menus |
| src/app/map/[id]/components/controls/DataSourceItem.tsx | Added inline renaming and context menu actions |
| src/app/map/[id]/components/controls/LayerEmptyMessage.tsx | Enhanced with button mode and dropdown support |
| src/app/map/[id]/components/controls/VisualisationPanel/* | Added IncludeColumnsModal and CategoryColorEditor |
| src/app/map/[id]/components/Legend.tsx | Redesigned with category swatches and visibility toggle |
| src/app/map/[id]/colors.ts | Added categoryColors support for custom categorical colors |
| src/components/icons/VectorSquare.tsx | Removed custom icon (replaced with lucide-react) |
| package.json | Updated lucide-react from 0.484.0 to 0.562.0 |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| .filter(({ name, type }) => { | ||
| // Must be a number column | ||
| if (type !== ColumnType.Number) return false; | ||
|
|
||
| // If includeColumns is specified, only include those columns | ||
| if (includeColumns && includeColumns.length > 0) { | ||
| return includeColumns.includes(name); | ||
| } | ||
|
|
||
| // Otherwise, exclude columns in excludeColumns list | ||
| return !excludeColumns.includes(name); | ||
| }) |
Copilot
AI
Jan 15, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
When includeColumns is specified with length > 0, the excludeColumns list is completely ignored. This could lead to unexpected behavior if a user expects both include and exclude to work together. Consider either documenting this behavior clearly or handling the case where both are provided (e.g., by applying excludeColumns as a filter after includeColumns).
| "kysely": "^0.27.6", | ||
| "lucide": "^0.544.0", | ||
| "lucide-react": "^0.484.0", | ||
| "lucide-react": "^0.562.0", |
Copilot
AI
Jan 15, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The lucide-react package is being updated from version 0.484.0 to 0.562.0. This is a significant jump that may introduce breaking changes or new icons (like VectorSquareIcon which is being used in this PR). Ensure that all icon imports are compatible with the new version and check the lucide-react changelog for any breaking changes.
| typeof input.autoEnrich === "boolean" || | ||
| typeof input.autoImport === "boolean"; |
Copilot
AI
Jan 15, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The condition for checking if autoEnrich and autoImport have changed uses typeof to check if they're boolean, but this doesn't verify if the values actually changed from their previous state. This means any update that includes these fields (even if unchanged) will trigger a re-import. Consider comparing against the previous values to only trigger import when there's an actual change.
| <p className="text-sm text-gray-500 mb-4"> | ||
| Only selected columns will be considered when determining the | ||
| highest value column for each area. Leave empty to use all numeric | ||
| columns. |
Copilot
AI
Jan 15, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The IncludeColumnsModal description states "Leave empty to use all numeric columns", but this conflicts with the actual behavior where an empty includeColumns results in using all columns except those in excludeColumns. The UI text should be updated to clarify that when no columns are selected, the excludeColumns filter is applied instead.
| enableVisibilityToggle={Boolean( | ||
| placedMarkers.length > 0 || | ||
| markerDataSources.length > 0 || | ||
| membersDataSource, | ||
| )} |
Copilot
AI
Jan 15, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The condition checking if the layer should be enabled uses Boolean(placedMarkers.length > 0 || ...) which is redundant since the expression already evaluates to a boolean. The outer Boolean() call is unnecessary and can be removed for cleaner code.
| enableVisibilityToggle={Boolean( | |
| placedMarkers.length > 0 || | |
| markerDataSources.length > 0 || | |
| membersDataSource, | |
| )} | |
| enableVisibilityToggle={ | |
| placedMarkers.length > 0 || | |
| markerDataSources.length > 0 || | |
| membersDataSource | |
| } |
| > | ||
| {/* Fill Layer - only show for choropleth */} | ||
| <Layer | ||
| key={`${sourceId}-fill-${viewConfig.areaDataColumn}-${viewConfig.calculationType}-${viewConfig.colorScheme}`} |
Copilot
AI
Jan 15, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The Layer key includes categoryColors in its dependencies via viewConfig.categoryColors, but categoryColors is an object/record. Since the key is a string template, changes to categoryColors won't be reflected in the key and the layer won't re-render when category colors change. Consider adding a serialized version of categoryColors to the key or using a different approach to force layer updates when colors change.
| if (dropdownItems) { | ||
| return ( | ||
| <MultiDropdownMenu | ||
| dropdownLabel="Marker options" |
Copilot
AI
Jan 15, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The dropdownLabel is hardcoded to "Marker options" in the AddLayerButton component, but this component is used for different layer types (turfs, boundaries, markers). This makes the dropdown label incorrect when used with non-marker layers. Consider making dropdownLabel a parameter or deriving it from the context.
| const configFieldsChanged = | ||
| input.columnRoles !== undefined || | ||
| input.enrichments !== undefined || | ||
| input.geocodingConfig !== undefined || | ||
| input.dateFormat !== undefined || | ||
| input.public !== undefined || | ||
| typeof input.autoEnrich === "boolean" || | ||
| typeof input.autoImport === "boolean"; |
Copilot
AI
Jan 15, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The condition configFieldsChanged checks if fields are !== undefined, but this means passing undefined explicitly won't trigger a re-import, while passing null or any other value will. This could lead to confusing behavior. Consider using in operator to check if the property exists in the input object, or be more explicit about what constitutes a change.
| typeof input.autoEnrich === "boolean" || | ||
| typeof input.autoImport === "boolean"; | ||
|
|
||
| if (configFieldsChanged) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
note: remove public, autoenrich, autoimport from the check here
| column: z.string(), | ||
| secondaryColumn: z.string().optional(), | ||
| excludeColumns: z.array(z.string()), | ||
| includeColumns: z.array(z.string()).optional().nullable(), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
note: remove excludeColumns
Co-authored-by: Copilot <[email protected]>
Co-authored-by: Copilot <[email protected]>
…com/commonknowledge/ts-mapped into layers-panel/jan26-updates-shorterm
…dColorEditor.tsx Co-authored-by: Copilot <[email protected]>
…isationPanel.tsx Co-authored-by: Copilot <[email protected]>
…dColorEditor.tsx Co-authored-by: Copilot <[email protected]>
…isationPanel.tsx Co-authored-by: Copilot <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Copilot encountered an error and was unable to review this pull request. You can try again by re-requesting a review.
…com/commonknowledge/ts-mapped into layers-panel/jan26-updates-shorterm
…onknowledge/ts-mapped into layers-panel/jan26-updates-shorterm
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
Copilot reviewed 41 out of 42 changed files in this pull request and generated 7 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| const configFieldsChanged = | ||
| input.columnRoles !== undefined || | ||
| input.enrichments !== undefined || | ||
| input.geocodingConfig !== undefined || | ||
| input.dateFormat !== undefined; | ||
|
|
||
| if (configFieldsChanged) { | ||
| await enqueue("importDataSource", ctx.dataSource.id, { | ||
| dataSourceId: ctx.dataSource.id, | ||
| }); | ||
| } |
Copilot
AI
Jan 16, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The logic check for configFieldsChanged only checks if the fields are not undefined, but doesn't verify if they've actually changed from their previous values. This means that even if a user updates the name and then clicks save without changing config fields, but the config fields happen to be defined, the import will not be triggered. Consider comparing the new values with existing values to determine if a re-import is truly necessary.
| map.setFeatureState( | ||
| { | ||
| source: sourceId, | ||
| sourceLayer: layerId, | ||
| id: areaCode, | ||
| }, | ||
| { value: null, secondaryValue: null }, | ||
| ); |
Copilot
AI
Jan 16, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The function removeFeatureState has been changed to setFeatureState with null values. While this should work, verify that this is the intended behavior change. The comment says "Remove lingering feature states" but the implementation now sets them to null rather than removing them. Ensure this doesn't cause any unexpected behavior with Mapbox GL.
| ): AreaSetGroupCode[] => { | ||
| if (!dataSourceGeocodingConfig) { | ||
| return []; | ||
| return Object.values(AreaSetGroupCode); |
Copilot
AI
Jan 16, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
When no geocoding config is provided, the function now returns all AreaSetGroupCodes instead of an empty array. This is a significant behavior change that could allow users to select area sets that aren't compatible with their data source. This might lead to incorrect visualizations or errors when geocoding is attempted. Verify this change is intentional and that there are proper safeguards elsewhere in the code.
| return Object.values(AreaSetGroupCode); | |
| return []; |
| className="flex items-center gap-2 w-full min-h-full p-1 rounded transition-colors hover:bg-neutral-100 text-left cursor-pointer" | ||
| onClick={() => handleFlyTo(turf)} |
Copilot
AI
Jan 16, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The styling classes use forward slashes (/) as separators which is unusual and might not be valid Tailwind CSS syntax. For example: "flex items-center gap-2 / w-full min-h-full p-1 rounded / transition-colors hover:bg-neutral-100 / text-left cursor-pointer". This should use spaces only. Verify if this is intentional or if these are typos that could cause styling issues.
| ref={isDraggingMarker ? setHeaderNodeRef : null} | ||
| onClick={() => onClickFolder()} | ||
| className={cn( | ||
| "flex items-center gap-1 / w-full min-h-full p-1 rounded / transition-colors hover:bg-neutral-100 / text-left cursor-pointer", |
Copilot
AI
Jan 16, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Similar issue with forward slashes in className strings that appear to be separators. These should be removed as they're not valid Tailwind CSS syntax: "flex items-center gap-1 / w-full min-h-full p-1 rounded / transition-colors hover:bg-neutral-100 / text-left cursor-pointer". This pattern appears in multiple places and should be cleaned up.
| "flex items-center gap-1 / w-full min-h-full p-1 rounded / transition-colors hover:bg-neutral-100 / text-left cursor-pointer", | |
| "flex items-center gap-1 w-full min-h-full p-1 rounded transition-colors hover:bg-neutral-100 text-left cursor-pointer", |
| @@ -1,5 +1,4 @@ | |||
| import { ChartBar, MapPin } from "lucide-react"; | |||
| import VectorSquare from "@/components/icons/VectorSquare"; | |||
| import { ChartBar, MapPin, VectorSquareIcon } from "lucide-react"; | |||
Copilot
AI
Jan 16, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The VectorSquareIcon icon may not exist in lucide-react. The PR updates lucide-react to version 0.562.0, and while this version exists, VectorSquareIcon might not be the correct icon name. In the previous code, a custom VectorSquare component was used. Please verify that VectorSquareIcon is a valid icon in lucide-react 0.562.0, otherwise this will cause a runtime error.
| MapPinIcon, | ||
| SquareIcon, | ||
| UsersIcon, | ||
| VectorSquareIcon, |
Copilot
AI
Jan 16, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The VectorSquareIcon may not exist in lucide-react. A similar issue exists here where the code references VectorSquareIcon from lucide-react. Please verify this is a valid icon name in version 0.562.0, or use the correct icon name to avoid runtime errors.
-add/remove layers
Description
Motivation and Context
How Can It Be Tested?
How Will This Be Deployed?
Screenshots (if appropriate):
Types of changes
Checklist: